Hi Ky,

thanks a lot for your contribution! Having all changes in one big pull request, makes it hard to review, so it would be great if you could split this up and do a little cleanup as well. Just tell me, if you need help with it.

Cheers Jochen

What is generally the recommended way to do that? Git rebase seems to be the desired method, but everyone seems to warn against rebasing commits already pushed to a public server.

(I haven't done much work with rewriting history before.)

Rebasing is fine as long as no one else is sharing your history. For PCL the PointCloudLibrary/pcl repo is the public one, everyone uses contrary to your fork, which no one should use without being aware that it's work in progress.

I refactored much of the history, but some of the log messages are still useless (even to me). I will not have time to do more clean up until after ISMAR is over, so it might be better to leave integration pending until then.

Okay, I squashed most of the commits into cohesive units for review.

HI @kwaegel could you please remove the merge commit? Thanks!

How do I remove the merge commit? I rebased on master as part of my cleanup, but I'm not sure why that commit is listed as part of my branch.

I looked into combining the nearly identical files in openni_camera and openni2_camera, but have not had time to do that yet.

Switching entirely to OpenNI2 would be nice, but would add a dependency (OpenNI2-Freenect) since OpenNI2 does not have native Kinect support on Linux.

Removed merge commit by rebasing on current master again.

Can you run? I can not run pcl_openni2_viewer after.cmake your openni2 of branchs.
openni2_grabber.cpp - > boost::this_thread::sleep (boost::posix_time::seconds (1)); run here .Interrupt

Try the openni2_pullrequest branch. The openni2 branch can be a little unstable, though I would not have expected that particular problem.

@jspricke What is a good way to add unit tests for what is essentially a device driver? Could we use a tiny (~5 frame) depth camera recording for automated testing? OpenNI 2 wraps devices and device recordings with the same interface. I'm not sure if data files are appropriate for the repository.

• Ky Waegel notifications@github.com [2013-10-23 15:50]:

  @jspricke What is a good way to add unit tests for what is essentially a device driver? Could we use a tiny (~5 frame) depth camera recording for automated testing? OpenNI 2 wraps devices and device recordings with the same interface. I'm not sure if data files are appropriate for the repository.

I wouldn't add tests, as we would test OpenNI2 basically. Maybe it would
make sense to test stuff like the Bayer algorithm (is this still needed
btw.?) but that's just a copy and paste from OpenNI1..

The pull request cannot be merged, can you please fix it?

I can rebase on master again, but I'll wait until my win32_viz_fixes pull-request is integrated. I can't compile the master branch otherwise.

I tried building your pull request branch (kwaegel/openni2_pullrequest) but failed. It seems like the culprit was the auto keyword used in openni2_device.cpp

/pcl/io/src/openni2_camera/openni2_device.cpp:570:5: warning: ‘auto’ changes meaning in C++11; please remove it [-Wc++0x-compat]
/pcl/io/src/openni2_camera/openni2_device.cpp:570:10: error: ‘modeList’ does not name a type
auto modeList = getSupportedDepthVideoModes();
    ^

This was using GCC on Linux (x64, Fedora 19).

I am a bit surprised, because I tried the same branch two days ago (Saturday 14th), and then I had no problems.

That's from the most recent commit (68b4e79) fixing the default video mode. I used the C++11 definition of auto, but apparently it's not safe to do so yet. I'll have to change it back.

Great! I got it working with pcl_kinfu_app as well. Hope to see this merged soon.
By the way, is there a (good) reason why BUILD_OPENNI and BUILD_OPENNI2 have to be mutually exclusive?

That's great that it works on Linux! Did you have to change FindOpenNI2.cmake at all?

I made BUILD_OPENNI and BUILD_OPENNI2 mutually exclusive just to avoid potential conflicts, since I don't have a good way to test the two alongside each other. There is an explicit check in OpenNI 1.x against compiling with VS2012 (my build platform).

I'd like to have a common interface so we can use something like HAS_OPENNI_GRABBER when building dependent projects, regardless of which version it found, but that might preclude building both at once.

Nope, no changes to FindOpenNI2.cmake, but I did specify the OPENNI2_* variables directly. I never installed OpenNI2 to my system, but kept the Redist directory. I have so far failed to find an automatic way to install OpenNI2 on Linux (which is sad...).

I personally am fine with having it mutually exclusive for now.

I'm not entirely sure if they should be mutually exclusive. I'll try to find some time together with @sdmiller and test this, as it's a pretty important feature to have in PCL 1.8

Unfortunately this pull request can no longer be merged automatically. Can we please fix the conflict?

@kwaegel Why did you close the pull request?

Needed to be rebased on the PCL master branch again, since it was about two months behind and could not auto-merge.

OK, I can confirm that it builds on Linux without any errors (although there are quite a few warnings). I had some other problem, on the other hand: I couldn't make it to build the OpenNI2 grabber. Both in command line and cmake-gui listed that OpenNI2 exists and was found. I have no idea why it happened. Did I forget to change some flag somewhere (I know only two CMakeLists files that provide installation options for OpenNI)? In the end, I just commented out those lines which were testing whether or not OpenNI2 exists. I know that this is no solution, but unfortunately I wasn't able to spend more time on this issue. The main thing is, that it builds and runs (tested Xtion camera).

@mtmal:

• Most of the errors have been fixed on Linux.
• I haven't tried the instillation yet target yet, so I'm not surprised it's broken.
• Common files (io_exception.h) are already in pcl/io

I've also started building this on Clang in a VM, to avoid future compile errors, and I've not had any problems building the 2.x grabber.

@jspricke: I used the io/openni2 folder to match the namespace pcl::io::openni2. Something is not going to match whatever the directory is named, since the older grabber uses a completely different namespace convention.

Ah, openni2 is fine then :).

Hi, I would propose to merge this soon and fix the remaining issues with new pull requests. What do you think @rbrusu, @taketwo, @kwaegel.

I'm not in touch with this part of PCL, but in general I think merging won't hurt, but only help to discover bugs faster.

I see that this PR changes the required version of Boost to 1.47. I checked the downloads section of pointclouds.org and it seems like we already provide "right" versions for Windows and Mac. "Compiling from source" page talks about 1.46 though.

@kwaegel looks like there is a merge conflict. Can you fix this, so we can push the button? ;). Thanks!

+1. OpenNI2 support is an important contribution.

@jochen, will do, but it might take up to a week. I'm at the VR 2014
conference right now and without access to my dev system.
On Mar 29, 2014 9:55 AM, "Jochen Sprickerhof" notifications@github.com
wrote:

@kwaegel https://github.com/kwaegel looks like there is a merge
conflict. Can you fix this, so we can push the button? ;). Thanks!

Reply to this email directly or view it on GitHubhttps://github.com//pull/276#issuecomment-38997802
.

Fixed the merge conflicts, rebased on master, and squashed some of the minor fixup commits.

I noticed there are a few places where a space is missing before braces. Also the headers with copyright are dissonant. Many new files have Willow Garage instead of Open Perception.

Otherwise looks good to me. @rbrusu @jspricke Shall we finally merge?

+1